Skip to content

[repo-assist] fix: correct string enum array serialisation in toQueryParams; add enum query-param tests#462

Merged
sergey-tihon merged 4 commits into
masterfrom
repo-assist/perf-test-enum-queryparams-20260620-1841bb28be3b9fb0
Jun 21, 2026
Merged

[repo-assist] fix: correct string enum array serialisation in toQueryParams; add enum query-param tests#462
sergey-tihon merged 4 commits into
masterfrom
repo-assist/perf-test-enum-queryparams-20260620-1841bb28be3b9fb0

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

🤖 This PR was created by Repo Assist, an automated AI assistant.

Summary

This PR fixes a latent bug in toQueryParams and adds test coverage that surfaced it, plus a small performance improvement to createHttpRequestFromQueryLists.


Bug fix: string enum arrays serialised as integers

Root cause

In .NET the CLR applies array variance for enum types: any array of an int32-backed enum (e.g. Status[]) can be matched by a | :? int32[] pattern. In toQueryParams the concrete | :? array<int32> arm therefore fired for every such enum array — before the generic Array guard that was meant to handle enums.

For integer enums this gave the right answer by accident (the integer string).
For string enums (annotated with [<JsonConverter(typeof<JsonStringEnumConverter>)>]) it silently discarded the wire-name lookup and returned the underlying integer instead:

// Before: Status.Active (= 0) in an array → "0"  ❌
// After:  Status.Active (= 0) in an array → "active"  ✓

Fix

Add a dedicated enum-array arm that fires before all the concrete array<T> arms in toQueryParams:

| :? Array as xs when (let et = xs.GetType().GetElementType() in not(isNull et) && et.IsEnum) ->
    let serializer = enumSerializerCache.GetOrAdd(elTy, enumSerializerFactory)
    [ for i in 0..xs.Length-1 do
          let param = serializer(xs.GetValue(i))
          if not(isNull param) then yield name, param ]

This uses the cached buildEnumSerializer per element type, so both integer and string enums are serialised correctly. The existing | :? Array fallback for DateOnly/TimeOnly arrays is unchanged.


Performance: createHttpRequestFromQueryLists (Task 8)

Replaced the explicit ResizeArray accumulation loop with Seq.concat, which lazily flattens the nested sequences without an intermediate heap allocation. createHttpRequest already filters nulls in its own loop, so the redundant null check was also removed.

// Before
let queryParams = ResizeArray<string * string>()
for queryParamList in queryParamLists do
    for name, value in queryParamList do
        if not(isNull value) then queryParams.Add(name, value)
createHttpRequest httpMethod address queryParams

// After
createHttpRequest httpMethod address (Seq.concat queryParamLists)

New tests (5)

All in ToQueryParamsTests:

Test Covers
single integer enum value toQueryParamstoParam → integer serialiser
single string enum value toQueryParamstoParam → wire-name serialiser
string enum with special-character wire name "in-progress" round-trip
integer enum array new enum-array arm, integer serialiser
string enum array new enum-array arm, wire-name serialiser ← was broken before

Test Status

  • Build: ✅ 0 errors, 0 new warnings
  • Unit tests: ✅ 517 passed, 0 failed, 1 pre-existing skip (Schema_ParserTests.Fail to parse — no samples)
  • Format: ✅ Fantomas check passes

Generated by 🌈 Repo Assist, see workflow run. Learn more.

Add this agentic workflows to your repo

To install this agentic workflow, run

gh aw add githubnext/agentics/workflows/repo-assist.md@e15e57b40918dbca11b350c55d02ab61934afa75

…um query-param tests

toQueryParams had a silent bug: any array of an int32-backed enum type (e.g. Status[])
matched the '| :? array<int32>' arm via CLR array variance before the generic Array arm
could run.  For integer enums this happened to produce the right result (the integer
string), but for string enums (annotated with JsonStringEnumConverter) it discarded the
wire-name lookup and returned the underlying integer instead ('0' instead of 'active').

Fix: add a dedicated enum-array arm that fires before all the concrete array<T> arms.
It uses the cached buildEnumSerializer per element type so both integer and string enums
are serialised correctly.

Also simplifies createHttpRequestFromQueryLists by replacing the explicit ResizeArray
accumulation loop with Seq.concat, which lazily flattens without an intermediate heap
allocation (createHttpRequest already filters nulls in its own loop).

New tests in ToQueryParamsTests:
- single integer enum value
- single string enum value
- string enum value with special-character wire name (e.g. 'in-progress')
- integer enum array
- string enum array  ← previously produced wrong integer strings

Test count: 512 → 517 (5 new).  All 517 pass; 1 pre-existing skip unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@sergey-tihon sergey-tihon marked this pull request as ready for review June 20, 2026 06:47
Copilot AI review requested due to automatic review settings June 20, 2026 06:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes query-parameter serialization for enum arrays in the runtime helpers, and adds unit tests to cover both integer and string-enum query params (including arrays), plus a small change to query-param list flattening when building HTTP requests.

Changes:

  • Update toQueryParams to correctly serialize enum arrays using the cached enum serializer (preserving string-enum wire names).
  • Add unit tests covering single enum values and enum arrays (integer + string enums, including special-character wire names).
  • Simplify createHttpRequestFromQueryLists by flattening via Seq.concat and relying on createHttpRequest for null filtering.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
tests/SwaggerProvider.Tests/RuntimeHelpersTests.fs Adds toQueryParams tests for enum query params (single values + arrays).
src/SwaggerProvider.Runtime/RuntimeHelpers.fs Fixes enum-array handling in toQueryParams and simplifies query-param flattening for request creation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/SwaggerProvider.Runtime/RuntimeHelpers.fs
Comment thread tests/SwaggerProvider.Tests/RuntimeHelpersTests.fs Outdated
sergey-tihon and others added 2 commits June 21, 2026 08:24
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@sergey-tihon sergey-tihon merged commit ec59310 into master Jun 21, 2026
2 checks passed
@sergey-tihon sergey-tihon deleted the repo-assist/perf-test-enum-queryparams-20260620-1841bb28be3b9fb0 branch June 21, 2026 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants